feat: add scoped subject-turn submission - #468

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Summary

This PR adds a scoped POST /v1/runs/{run_id}/subject-turns endpoint that lets a running automation script submit work for an external subject (issue, PR, ticket) without receiving conversation credentials or managing runtime lifecycle. The service owns conversation identity, profile selection, and runtime attachment. The implementation reuses existing subject locking, coalescing, deterministic conversation IDs, and the agent-turn run machinery from #467.

Analysis

Security model is sound. The run token is a short-lived (24h) HS256 JWT scoped to subject_turn:submit and bound to a specific automation_id + run_id. The router validates the bearer token, checks scope, verifies run_id and automation_id match the URL path and DB record, and requires the requester run to be RUNNING. The script inside the sandbox cannot forge tokens for other runs or automations, and cannot choose an arbitrary conversation ID.

Idempotency and retry logic is well-designed. The unique constraint on (automation_id, source, subject_key, idempotency_key) prevents duplicates at the DB level. The transaction-scoped advisory lock (pg_advisory_xact_lock) serializes concurrent submissions for the same subject, so the SELECT-then-INSERT idempotency check is race-free. The retry path correctly releases failed/skipped runs from the subject lookup (subject_released_at) before creating a replacement, and reuses the deterministic conversation ID so the new attempt continues the same conversation. The single idempotency record is repointed to the new attempt while the superseded run remains in history.

Migration is cross-database compatible. Uses generic sa.Uuid, sa.String, sa.DateTime types. The unique constraint creates an implicit index that covers the idempotency lookup query. The test_migrations_run_on_sqlite test is updated.

Test coverage is strong. 9 test functions (11 with parameterization) cover first-turn creation, deduplication, retry of released failed/canceled/skipped runs, retry of a skipped run that never started, waiting for a failed run that hasn't released, source isolation, token-for-another-run rejection, endpoint acceptance, and continuation without a second run. The tests exercise real DB state and assert on outcomes, not just mocked calls.

Minor Observations (non-blocking)

1. No test for the "queued" disposition. The queued path (turn coalesced into a not-yet-started run) is not directly tested. It's exercised indirectly through _continue_conversation_locked's coalescing logic, but a test that submits two turns for the same subject before the first run is dispatched would close the gap.

2. Token signing key reuse. signing_secret() falls back to service_key (used for per-user API key minting) or local_api_key. The KV token uses a separate kv_secret. Key separation would be marginally better, but both secrets are server-side and never exposed to users, so the risk is theoretical. This follows the same pragmatic pattern as other service-level operations.

Risk Assessment

🟢 LOW — The change adds a new endpoint and table without modifying existing behavior. The dispatcher injection is additive (wrapped in try/except, silently disabled when no secret is configured). The security model is properly scoped. Race conditions are handled with advisory locks and unique constraints. No breaking changes to existing APIs.

Verdict

✅ Worth merging — The design is clean, the security model is sound, and the retry/idempotency logic is correct. The minor testing gap for the "queued" disposition is non-blocking.

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Summary

This PR adds a scoped POST /v1/runs/{run_id}/subject-turns endpoint that lets a running automation (scanner) submit conversation-scoped work for an external subject. The design is clean and well-structured: JWT-based run tokens with a single scope, a dedicated idempotency table with a unique constraint, and reuse of the subject locking/coalescing infrastructure from #467.

Taste Rating: 🟢 Good taste — the separation of concerns (scanner selects work, service owns conversation identity/runtime/serialization) is the right abstraction, and the idempotency + retry model handles the failure cases that matter.

Analysis

Authorization

The run token model is sound: HS256 JWT scoped to (automation_id, run_id, "subject_turn:submit") with 24h expiration. The router verifies token scope, run identity, automation identity, and RUNNING status before proceeding. The scanner receives no conversation credentials and cannot control conversation IDs. The signing_secret fallback (service_key or local_api_key) is acceptable — in cloud mode the service key is already the highest-privilege secret, so using it for JWT signing doesn't expand the attack surface, and the tokens themselves are low-privilege.

Idempotency and Retry Semantics

The automation_subject_turns table with its unique constraint on (automation_id, source, subject_key, idempotency_key) correctly prevents duplicate work. The retry logic is well-reasoned:

• Queued/running/successful → deduplicated
• Failed/cancelled/skipped + released → retried in the same deterministic conversation
• Failed + not yet released → deduplicated (waits for release)

The handling of runs that never started (started_at is None) is particularly well done — explicitly setting subject_released_at before retrying to ensure _lock_subject_run excludes the superseded run.

Migration

Cross-database compatible: uses sa.Uuid, sa.String, sa.DateTime(timezone=True), sa.text("CURRENT_TIMESTAMP"). No PostgreSQL-specific types. Chain is correct (028 → 027).

Tests

Comprehensive coverage: first-turn creation, idempotency dedup, queued coalescing, retryable terminal outcomes, unreleased-runtime protection, source isolation, token authorization, and continuation without a second run. Tests exercise real code paths with a real database session.

Non-blocking observation

The PR description states "Retry failed, canceled, skipped, or timed-out work." Timed-out runs are marked FAILED by the watchdog, which does not set subject_released_at for conversation-scoped runs (the watchdog code has no subject-awareness — this is a pre-existing gap from the base branch, not introduced by this PR). Consequently, a submit_subject_turn retry with the same idempotency_key against a watchdog-timed-out run would deduplicate rather than retry, since can_retry requires subject_released_at is not None when started_at is set.

In practice this is mitigated: a new idempotency_key for the same subject goes through _continue_conversation_locked, which attempts delivery, fails (runtime gone), and releases the subject — unblocking subsequent retries. The live evidence in the PR description confirms retries work in the assembled stack. Flagging only for awareness; no change needed in this PR.

Risk Assessment

[RISK ASSESSMENT]

• Overall PR: 🟢 LOW
  The endpoint is scoped, token-authenticated, and idempotent. No new external dependencies (pyjwt already declared). The migration is straightforward and cross-database compatible. The retry logic correctly handles the cases where subject_released_at is set, and the self-healing delivery path covers the gap for new idempotency keys.

VERDICT: ✅ Worth merging

KEY INSIGHT: The least-privilege token model — giving the scanner only subject-turn submission for its own run, never conversation credentials or ID control — is the right security boundary for fan-out from a scanner to multiple independent conversations.

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Summary

This PR adds a scoped POST /v1/runs/{run_id}/subject-turns endpoint that lets a running scanner fan out conversation-scoped work for external subjects, with idempotency, coalescing, deterministic conversation identity, and retry of terminal outcomes. The design is clean: the service owns conversation identity and runtime lifecycle; the scanner receives only a short-lived, single-scope capability token.

Analysis

Idempotency and retry logic — submit_subject_turn correctly serializes on the transaction-scoped subject lock before checking the idempotency record, so concurrent retries cannot create two conversations. The retry condition (status in _RETRYABLE and (started_at is None or subject_released_at is not None)) properly distinguishes between a run that never dispatched (safe to retry immediately) and one whose runtime must be released first. Reusing the existing idempotency record (retry_record.subject_run_id = subject_run.id) instead of inserting a new row avoids unique-constraint conflicts and preserves a single audit trail.

Capability token — The run token is scoped to subject_turn:submit, bound to a specific automation_id and run_id, and expires in 24h. The endpoint verifies both the scope and the run/automation binding before touching the database. The scanner never receives conversation credentials or control over conversation IDs. This is a sound least-privilege design.

Dispatcher refactor — The _release_conversation_subject closure consolidates subject release across all failure paths (concurrency limit, context fetch failure, conversation turn failure). The change to only release the subject after backend.release_context succeeds (line 431) is correct: if the runtime can't be released, marking the subject as released would orphan it.

Migration — Cross-database compatible: uses generic sa.Uuid() and sa.String() types, no PostgreSQL-specific features.

Non-blocking observation

The watchdog (watchdog.py) marks timed-out or crashed conversation-scoped runs as FAILED but does not set subject_released_at. When the dispatcher's own timeout handling fires (the effective_timeout passed to run_conversation_turn), the subject is released correctly. But if the dispatcher process itself crashes, the watchdog recovery path leaves subject_released_at NULL, and submit_subject_turn's retry check will return deduplicated instead of retrying. This is a narrow edge case (process crash during conversation turn execution), and the normal failure/timeout paths all release correctly. Consider having the watchdog set subject_released_at when it marks a conversation-scoped run terminal, as a follow-up.

Test coverage

Tests cover first-turn creation, idempotency deduplication, queued coalescing, retry of released FAILED/CANCELLED/SKIPPED runs, retry of never-started skipped runs, unreleased-runtime protection (deduplicated), source isolation, token-run binding (403), token automation binding, and continuation delivery. The dispatcher test for context-failure subject release is a good addition. Coverage is thorough.

[RISK ASSESSMENT]

• Overall PR: 🟢 LOW. The change adds a new scoped endpoint with proper auth, locking, and idempotency. No existing behavior is altered — the dispatcher refactor preserves all existing release semantics. The signing secret gracefully degrades (no token injected if no secret configured).

VERDICT: ✅ Worth merging. Core logic is sound, idempotency and locking are correct, and the least-privilege token design is well-considered.

KEY INSIGHT: The subject lock + idempotency record combination ensures that retries are deterministic: one retry produces exactly one new run in the same derived conversation, never two.

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Summary

This PR adds a scoped POST /v1/runs/{run_id}/subject-turns endpoint that lets a running scanner fan out conversation-scoped work for external subjects. The design is sound: the service owns conversation identity and runtime lifecycle; the scanner receives only a short-lived, single-scope capability token. Idempotency, coalescing, deterministic conversation identity, and retry of terminal outcomes are all correctly implemented.

Material Finding

Subject stuck permanently when release_context fails after a conversation turn failure (dispatcher.py, line 434)

The refactored error handling gates subject_released_at on backend.release_context succeeding. If the conversation turn fails and release_context also fails -- which is the likely scenario when the sandbox/agent server is already dead or unreachable -- the subject is never released. The run is marked FAILED, but subject_released_at stays NULL.

This means submit_subject_turn's retry check (can_retry = status in _RETRYABLE and (started_at is None or subject_released_at is not None)) returns False, and the subject is permanently deduplicated -- the exact problem this PR set out to solve ("Permanently deduplicating failed, canceled, skipped, or timed-out work can leave an external issue or PR stuck forever").

The watchdog cannot recover this either: it only processes RUNNING runs, and this run is already FAILED. No code path sets subject_released_at for a FAILED run whose release_context failed.

The previous code unconditionally set subject_released_at before attempting release_context. The new ordering is a regression for this failure mode. Consider releasing the subject unconditionally when the run is being marked FAILED (as the pre-runtime failure paths at lines 365 and 388 already do), or at minimum releasing it when release_context fails for a run that will be terminal anyway -- the runtime is effectively gone in that case.

[RISK ASSESSMENT]

• Overall PR: 🟡 MEDIUM. The idempotency, locking, and auth design are solid. The one material concern is the subject-stuck-forever path when release_context fails, which contradicts the PR's retry goal and requires operator intervention to recover.

VERDICT: Worth merging with a follow-up to address the release_context failure path -- or a quick fix to release the subject unconditionally on terminal failure, matching the pre-runtime failure paths.

KEY INSIGHT: The subject lock + idempotency record combination ensures deterministic retries, but only if subject_released_at is set on every terminal failure path -- the release_context-failed path is the gap.

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

What changed since the last review

The latest commit (533d9274, "fix: unblock failed subject retries") directly addresses the material finding from the prior review on d91fce1c: the subject was permanently stuck when both the conversation turn failed and backend.release_context also failed, because subject_released_at was gated on release_context succeeding.

Analysis of the fix

The conversation-turn failure path in dispatcher.py (lines 424-442) now wraps backend.release_context in its own try/except and calls _release_conversation_subject() unconditionally afterward - regardless of whether the runtime release succeeded or failed. This is the correct behavior: a run being marked FAILED has already lost its runtime, so the subject must be released to allow idempotent retries.

This matches the pre-runtime failure paths (lines 365-366, 387-388), which already release the subject unconditionally. All terminal failure paths for conversation-scoped runs now set subject_released_at.

The new test test_conversation_turn_failure_releases_subject_when_cleanup_fails (test_dispatcher.py, lines 307-370) verifies the exact scenario: submit_conversation_turn raises, release_context raises, and the test asserts finished.subject_released_at is not None. The existing test_conversation_turn_context_failure_releases_subject covers the pre-runtime path.

Full PR review

Security model - Sound. The run token is a short-lived (24h) HS256 JWT scoped to subject_turn:submit, bound to automation_id + run_id. The router verifies scope, run identity, automation identity, and RUNNING status before proceeding. The scanner receives neither conversation credentials nor control over conversation IDs.

Idempotency and retry - Correct. The transaction-scoped subject lock serializes the SELECT-then-INSERT idempotency check. The unique constraint on (automation_id, source, subject_key, idempotency_key) prevents duplicates at the DB level. The retry condition properly distinguishes never-started runs (releases immediately) from started-but-terminal runs (requires subject_released_at). The idempotency record is repointed to the new attempt rather than inserting a new row.

Migration - Cross-database compatible: generic sa.Uuid(), sa.String(), sa.DateTime(timezone=True). Chain 027 -> 028 intact.

Test coverage - Comprehensive: 12 tests covering first-turn creation, idempotency dedup, queued coalescing, retry of released FAILED/CANCELLED/SKIPPED runs, retry of never-started skipped runs, unreleased-runtime protection, source isolation, token authorization (403 for wrong run, 202 for correct), continuation delivery, pre-runtime context-failure subject release, and cleanup-failure subject release. All tests exercise real DB state.

Risk Assessment

[RISK ASSESSMENT]

• Overall PR: 🟢 LOW
  The endpoint is scoped, token-authenticated, and idempotent. The dispatcher change fixes a real failure mode (subject stuck when runtime cleanup fails) with matching test coverage. No existing behavior is altered - the dispatcher refactor preserves all existing release semantics while closing the gap. The signing secret gracefully degrades (no token injected if no secret configured). No new external dependencies (PyJWT already declared).

VERDICT: ✅ Worth merging. The previous material finding is resolved, and no new issues were found.

KEY INSIGHT: The unconditional subject_released_at on every terminal failure path - pre-runtime, runtime-gone, and cleanup-failed - ensures that the retry condition in submit_subject_turn can never be permanently blocked by a failed runtime release.

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your own agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.